Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SdSpiTeensy3.cpp - Use transfer(tx, rx, size) - avoid 512 bytes on stack #14

Merged

Conversation

KurtE
Copy link

@KurtE KurtE commented Jan 13, 2025

As I mentioned in a new issue, I created against the original code base for SdFat:
greiman#506

I noticed that the code for Teensy boards uses a special driver SdSpiTeensy3.cpp which uses
the SPI call: transfer(buffer, count);

Which on write functions, the code allocates 512 bytes on the stack, then copies the data out of the buffer passed in, to the copy on the stack, as to avoid having the transfer method overwriting the buffer passed in.

As you know, 8 years ago we added the alternative transfer method: transfer(txbuf, rxbuf, count); where either of these could be nullptr. Using this instead removes the need for the temporary copy on the stack.

Note: this is not the fix I would do, now, and from the comments by @greiman on the issue, he probably would not accept. His current released sources, now has the ability to do the exact same stuff without the need for a special driver. As the SdFatConfig.h file now has the ability to choose a few different ways to do these transfers without the need for a specialized driver:

/**
 * If USE_SPI_ARRAY_TRANSFER is one and the standard SPI library is
 * use, the array transfer function, transfer(buf, count), will be used.
 * This option will allocate a 512 byte temporary buffer for send.
 * This may be faster for some boards.  Do not use this with AVR boards.
 *
 * Warning: the next options are often fastest but only available for some
 * non-Arduino board packages.
 *
 * If USE_SPI_ARRAY_TRANSFER is two use transfer(nullptr, buf, count) for
 * receive and transfer(buf, nullptr, count) for send.
 *
 * If USE_SPI_ARRAY_TRANSFER is three use transfer(nullptr, buf, count) for
 * receive and transfer(buf, rxTmp, count) for send. Try this with Adafruit
 * SAMD51.
 *
 * If USE_SPI_ARRAY_TRANSFER is four use transfer(txTmp, buf, count) for
 * receive and transfer(buf, rxTmp, count) for send. Try this with STM32.
 */

We could simply use: #define USE_SPI_ARRAY_TRANSFER 2

The only thing this would miss, is the ability to pass in: that I am using the built-in SD adapter on T3.5/3.6/4.x to the SPI driver, and it will map that to the SPI pins on the adapter, which I have not seen nor heard of anyone using?

However, this updated code has not been merged into your fork, so went with this
limited update for now.

@PaulStoffregen
Copy link
Owner

This looks fine to me.

Maybe someday I'll bring all our changes into Bill's latest SdFat... but that day is not today, nor anytime soon.

@PaulStoffregen PaulStoffregen merged commit 68ae959 into PaulStoffregen:master Jan 16, 2025
@PaulStoffregen
Copy link
Owner

One quick question... has this been verified on Teensy 2.0?

I see we do have the 3-arg SPI.transfer() on Teensy 2.0, so my guess is this should probably be fine.

@KurtE KurtE deleted the SdSpiTeensy3_avoid_stack branch January 16, 2025 14:16
@KurtE
Copy link
Author

KurtE commented Jan 16, 2025

Maybe someday I'll bring all our changes into Bill's latest SdFat... but that day is not today, nor anytime soon.

I assumed that. As has been mentioned before, I would be nice if we could at least get to the point where having the teensy version, installed in /libraries does not interfere with using SDFat with other boards nor having the official SDFat installed does not interfere with building for Teensy.

One quick question... has this been verified on Teensy 2.0?

I did not try it... As I am pretty sure it is not used for Teensy 2.
At first because of the name of the file: SdSPITeensy3.cpp

But also by the #if in the file.
#if defined(SD_USE_CUSTOM_SPI) && defined(__arm__) && defined(CORE_TEENSY)
I would assume T2 does not define arm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants